Use __atomic builtins as the freelist 128-bit CAS fallback - #13571
Use __atomic builtins as the freelist 128-bit CAS fallback#13571phongn wants to merge 3 commits into
Conversation
moonchen
left a comment
There was a problem hiding this comment.
Clang/RISC-V still fails under the default warning-as-error build. With Clang 18, compiling these fallback operations using --target=riscv64-linux-gnu -march=rv64gc -Werror produces:
error: large atomic operation may incur significant performance penalty; the access size (16 bytes) exceeds the max lock-free size (8 bytes) [-Werror,-Watomic-alignment]
The diagnostic is emitted for both __atomic_load in INK_QUEUE_LD and __atomic_compare_exchange_n in the CAS specialization. The CMake probe can succeed because its try-compile does not inherit CMAKE_COMPILE_WARNING_AS_ERROR, but the actual C++ build then fails. Please suppress this expected diagnostic for the fallback path or otherwise avoid promoting it to an error.
Platforms without an inline 128-bit CAS, such as riscv64, fail the build with "unsupported processor". Neither GCC nor LLVM emit an inline 128-bit CAS on riscv64, even with the Zacas extension, so a hand-written pointer-packing branch would be the only alternative and would depend on the kernel's virtual address width. Fall back to the __atomic builtins instead. They lower to libatomic calls, which may take internal locks; that is correct because every access to a shared head_p goes through INK_QUEUE_LD and ink_atomic_cas. Also revive the orphaned atomic list stress test as Catch2 tests and remove the dead INK_QUEUE_NT code. Fixes: apache#13555 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2fc7cae to
f874981
Compare
moonchen
left a comment
There was a problem hiding this comment.
The -Werror failure is fixed -- confirmed with clang 18 --target=riscv64-linux-gnu -march=rv64gc: both sites error without the flag, warn with it. test_tscore passes ASAN-clean including the new [InkAtomicList] cases. One optional suggestion inline.
Co-authored-by: Mo Chen <uncorrupt@gmail.com>
Summary
ATS does not build on riscv64. The build stops in
ink_queue.hwith the error "unsupported processor" (#13555). The freelist head needs one of two mechanisms: a 128-bit CAS, or a hand-written pointer layout with version bits. riscv64 has neither mechanism.This change adds a portable fallback. When the 16-byte
__syncbuiltins are not available, the freelist uses the__atomicbuiltins. The__atomicbuiltins lower to libatomic calls. The build then links against libatomic.Why not wait for Zacas hardware
The Zacas extension does not help here:
amocas.qinstruction. The initial Zacas support deferred it, and it never landed.amocas.qfor ABI-compatibility reasons. See the LLVM RISC-V usage notes.Thus, on riscv64 the 16-byte atomic operations always go through libatomic. This is true on current hardware and on future Zacas hardware.
Why not a hand-written pointer layout
A packed 64-bit layout is possible, but it is fragile on RISC-V:
LogObject.ccuses the version field as a reference count, so few version bits are not safe.The
__atomicfallback has no address-space assumptions and keeps the full 64-bit version field. A packed riscv64 fast path can come later as an optimization if measurements justify it.Correctness
On riscv64, libatomic implements the 16-byte operations with internal locks. This is correct for this code because every access to a shared
head_pgoes throughINK_QUEUE_LDandink_atomic_cas. All of these operations serialize on the same libatomic lock, so the load/CAS retry loops keep their current semantics. The code comments now record this constraint.The
__syncpath stays first in the probe order. x86-64 and aarch64 builds produce the same code as before; GCC does not inline the 16-byte__atomicCAS even with-mcx16(GCC PR80878), so a blanket switch would regress them.Note: libatomic exports only
__atomic_*symbols. The 16-byte__syncbuiltins emit undefined__sync_*_16references when the compiler cannot inline them, and nothing provides those symbols. That is why the fallback must use the__atomicbuiltins, and why the two paths are mutually exclusive.Changes
cmake/Check128BitCas.cmake: probe the__atomicbuiltins when the__syncprobe fails, first without and then with-latomic. New variables:TS_HAS_128BIT_CAS_LIBATOMIC,TS_NEEDS_LIBATOMIC_FOR_CAS. GCC >= 14 removed the automatic-latomicon RISC-V, so the explicit link is necessary.ink_queue.h:INK_QUEUE_LDbecomes a 16-byte__atomic_loadon the fallback tier;head_pkeeps the plain{pointer, int64 version}layout there.ink_atomic.h: anink_atomic_cas<__int128_t>specialization uses__atomic_compare_exchange_non the fallback tier.src/tscore/CMakeLists.txt: linkatomic(PUBLIC) when needed.src/tscore/test_atomic.ccstress test was orphaned (no build system referenced it). It is replaced with Catch2 tests inunit_tests/test_InkAtomicList.cc: a concurrent push/pop/popall conservation test with a double-reachability detector, anink_atomiclist_removetest, and a concurrentInkFreeListnew/free test.INK_QUEUE_NTconditional (ink_queue_nt.cno longer exists) and report the new feature flag intraffic_layout.Performance
The fallback affects only platforms that had no working build before. A microbenchmark of the freelist access pattern (x86 proxy numbers): the lock-based path is ~1.7x slower than the inline CAS when uncontended (~37 ns vs ~21 ns per op), and equal or slightly faster under contention, because the futex parks waiters instead of burning CAS retries.
ProxyAllocatorthread caches absorb most freelist traffic, so the global head is not on the per-request fast path.Testing
test_tscorepass on x86-64 (__synctier, unchanged codegen).traffic_layout info --featuresshould showTS_HAS_128BIT_CAS_LIBATOMIC: 1there.Fixes: #13555
🤖 Generated with Claude Code